Skip to content

Correctness fixes, shared view lifecycle, and build gates - #10

Merged
stxkxs merged 3 commits into
mainfrom
feature-check
Aug 25, 2026
Merged

stxkxs merged 3 commits into
mainfrom
feature-check

Conversation

@stxkxs

@stxkxs stxkxs commented Aug 25, 2026

Copy link
Copy Markdown
Owner

Why

A quality audit graded this repo across ten dimensions and found five correctness defects, a
structural duplication that had already produced a shipped bug, and no CI. This branch closes all
of it. Writing the test suite the audit asked for then surfaced five more defects, which are fixed
here too — the first real test suite finding real bugs is the point of having one.

What changed

Three commits, one per concern.

f06a680 — engine correctness. One defect class ran through the engines: a player's display
name standing in where a stable id was needed. Names collide on NFL rosters and vary in spelling
across nflverse source columns, so grouping and joining on them produced plausible-looking wrong
numbers.

  • snap_share, waiver_wire windowed by name — two players sharing one merged into a single
    rolling series. A steady 90% snap share read as a 36.7% rolling average.
  • red_zone joined its receiving and rushing halves on the display name, splitting one GSIS id
    under two spellings into two rows — a dual-threat back counted at half his work.
  • data/stats merged its four stat lines on the display name, emitting one partial row per
    spelling.
  • injury_impact picked a team-season by iteration order over .unique(), which polars does not
    guarantee; identical input reported a different season run to run.
  • start_sit, trade_value returned a schema-less empty frame from their guard paths, so the
    first filter raised ColumnNotFoundError.
  • trade_value production multiplied PPG by a games-remaining term constant across players, so it
    cancelled in the normalisation that follows and could not affect any output. It is per player and
    bye-aware; the standalone bye component folds into it. Weights change to production 0.50,
    schedule 0.20, usage 0.20, health 0.10 — rankings move.
  • trade_value health scaled its denominator with how many seasons carried a report, so identical
    absences scored differently.
  • data/stats counted fumbles after filtering to pass/run, missing return fumbles, and omitted the
    position column entirely on the rosterless path the loader reaches by design.

1585e64 — shared lifecycle, data layer, gates. Six views each carried the same load
lifecycle; _on_data_error was byte-identical in all six. Three of the six Refresh buttons reached
the fetch with force_refresh=False, re-reading cache while reporting success. ToolView owns the
lifecycle so that divergence is unrepresentable, plus empty-state rendering, one filter-value
reader, and cancel/elapsed-time on long loads. Note Select.BLANK resolves to the inherited
Widget.BLANK — the bool False — so every guard using it was dead. Data layer gets atomic cache
writes with LRU eviction, per-download threads so a stall stops blocking later calls, and
date-injectable season resolution. CI runs lint, format, typecheck, coverage and a build-failing
vulnerability scan; mypy is strict = true with no carve-out.

10375af — docs. README documented a trade-value formula the engine does not implement and
CLAUDE.md named an importer that does not exist and a research file not in the repo. Adds
Configuration, Troubleshooting, Development, and docs/architecture.md.

How tested

Gate Result
ruff format --check . 0
ruff check . 0
mypy src (strict = true, no carve-out) 0
pytest --cov 0 — 423 tests, 86.07% vs floor 86
uv build 0
Warnings 0

Tests go from 13 to 423. One # type: ignore in src/, on the untyped nfl_data_py import, with
its error code. No test skipped or xfailed; the coverage gate was proven to fail when raised above
the measured total.

Every correctness fix was confirmed by running code, not by reading it — two same-named players
holding separate windows, guard-path frames filtering without raising, per-player production
varying with an upcoming bye, player_info stable across ten shuffled runs, one id under two
spellings yielding one red-zone row, identical absences scoring identical health, and the refresh
contract parametrised across all six views.

Reviewer notes

  • The trade-value weight change moves rankings. Bye week now costs a player proportionally one
    game out of the remaining slate rather than a flat 10 points off a 0–100 score, so mid-season
    byes are materially less punitive and the effect shrinks as the season runs out.
  • CI has never executed on a runner. Every step was run locally and exits 0, but cache
    restore, uv python install on ubuntu-latest and advisory-database reachability are unproven
    until the first push.
  • View coverage is the soft spot: injury_impact/screen.py at 57%, trade_value 74%,
    start_sit 75%. Engines and the data layer are well above the floor.
  • Follow-ups deliberately left out of scope: the fumble line groups by posteam, so a return
    fumble is charged to the kicking team's row; loader catches OSError broadly enough that a
    connection reset also triggers the play-by-play fallback; and _team_bye_weeks infers a bye from
    the first unscheduled week, so a partially loaded schedule would corrupt the production term that
    now depends on it.

stxkxs and others added 3 commits August 24, 2026 20:22
…the engines

The engines shared one defect class: a player's display name standing in where a
stable identifier was needed. Names collide on NFL rosters and vary in spelling
across nflverse source columns, so grouping and joining on them produced wrong
numbers that read as plausible.

─── Identity ───

snap_share, waiver_wire — rolling windows grouped by `player`, merging two people
who share a name into one series. Two "Josh Allen" rows, one steady at 90% snap
share and one at 10%, gave a 36.7% rolling average for the player who never left
90%. Both window over the player id; snap_share keeps `pfr_player_id` through
selection rather than discarding it.

red_zone — compute_player_rz full-joined its receiving and rushing halves on the
display name. nflverse fills receiver_player_name and rusher_player_name from
separate source fields, so one GSIS id under two spellings split into two rows and
rz_touches, rz_tds and td_pct each counted half the player's work — a dual-threat
back being exactly what the tool exists to surface. The join keys on the id and
coalesces a single display name through.

data/stats — compute_weekly_stats_from_pbp merged its four stat lines on
player_display_name, emitting one partial row per spelling for a player appearing
in several roles in a week. The merge keys on player_id alone.

injury_impact — teammates reduce with an explicit keep rather than polars'
arbitrary default, so a teammate whose listed position varies across weeks resolves
to a defined row. The searched player's team-season is chosen by sorting rather
than by iteration order over `.unique()`, which polars does not guarantee; the same
input had been yielding a different reported season run to run.

─── Empty results ───

start_sit, trade_value — the guard paths returned a bare pl.DataFrame() carrying no
columns, which raises ColumnNotFoundError the moment a caller filters it. Each
engine declares its output schema once at module level and builds both the
populated and the empty result against it, so the two cannot drift apart.

─── Scoring ───

trade_value — production multiplied season PPG by a games-remaining term constant
across players, so it cancelled exactly in the max-normalisation that follows and
could not influence any output. Games remaining is per player: regular-season
length less the current week, less one more when the team's bye is still ahead. The
standalone bye component folds into it and its weight redistributes, leaving
production 0.50, schedule 0.20, usage 0.20, health 0.10 — the bye counted once.

trade_value — the health denominator scaled with how many seasons carried an injury
report for a player, so two absences spread across two reported seasons scored
differently from the same two inside one. Every player is measured against the
seasons in the analysed window.

data/stats — lost fumbles aggregated from a frame already filtered to pass and run
plays, so a fumble on a kickoff or punt return never reached the -2 that the
published nflverse asset this function substitutes for does charge. Fumbles read
the unfiltered play-by-play.

data/stats — the derived frame carried a position column only when rosters were
supplied, while the loader reaches this path with rosters=None by design after a
roster fetch failure. start_sit and trade_value both filter on that column, so the
degraded path raised rather than degrading. The column is always present,
null-filled when rosters are absent.

red_zone — both results close on a total sort, so rows tying on the primary key
hold a reproducible order.

─── Tests ───

conftest carries a four-team, four-position league across two full 18-week seasons,
built so expected values are whole numbers a reader can verify by hand, alongside
colliding-name, injury, schedule and play-by-play frames. Season 2025 is pristine;
2024 holds one deliberate absence for injury_impact.

New suites cover trade_value, start_sit, injury_impact, red_zone and the
play-by-play derivation, none of which carried tests. snap_share and waiver_wire
gain the colliding-name case. Assertions are on computed values rather than frame
shapes, and thresholds are exercised at their boundaries.

Co-authored-by: stxkxsbot <275011021+stxkxsbot@users.noreply.github.com>
…he build

Six tool views each carried their own copy of the same load lifecycle. `activate`,
`_on_data_error`, `on_button_pressed` and `on_select_changed` appeared in all six,
`_populate_filters`, `_apply_filters` and `_fetch_data` in five, and the filter-bar
CSS block in all six; `_on_data_error` was byte-identical everywhere. Duplication
of a decision is duplication of the chance to get it wrong, and three of the six
Refresh buttons did.

─── ToolView ───

src/ffb/ui/base.py holds the lifecycle. A subclass sets ID_PREFIX and LOAD_LABEL
and implements compose_content, fetch and apply; the base owns the thread worker,
the loading pane, error reporting, refresh, filter reading and table rendering.

Refresh is the reason the seam exists. `on_button_pressed` matches the view's
refresh button and always calls the fetch with force_refresh=True. waiver_wire,
start_sit and trade_value had instead reset an activation flag and re-entered
`activate()`, reaching the fetch with force_refresh defaulting to False — so the
button re-read the same cache and reported success, against a README promising a
fresh download. Routing every view through one handler makes the divergence
unrepresentable.

`fill_table` renders an empty frame as a stated message rather than a bare header
over blank space, and reads emptiness from height so a frame carrying no columns at
all cannot raise. All six tools had rendered an empty result identically to a
loading one.

`selected` and `narrowing` collapse the unselected sentinel and the "All"
convention in one place. `Select.BLANK` resolves to the inherited `Widget.BLANK`,
the bool False, so every `is Select.BLANK` guard was dead and every
`not in ("All", Select.BLANK)` was true, letting a NoSelection reach a polars
comparison. `Select.NULL` is the sentinel, and it is reachable from one method.

A load renders once. Select option-setting and value assignment post `Select.Changed`
asynchronously, so a flag cleared when `apply` returns is already clear when those
messages dispatch and each triggers a further filter pass — a full recomputation per
filter in trade_value. The base compares live filter state against what backs the
current render and dispatches only on a real change.

`run_off_thread` owns the worker, the cancellation guard and error routing, so
injury_impact's player search shares the load worker's failure handling.

A long load shows what it is downloading and elapsed time, offers Cancel and binds
escape, and a cancelled load does not render. No percentage is displayed: nflverse
reports no progress, so one would be invented.

─── Data layer ───

loader — one `_cached` helper backs all seven loaders, replacing six copies of the
same cache-check-fetch-store body. Each download runs on its own daemon thread, so
a stalled fetch no longer holds the single shared worker while the next call's
timeout budget runs down against a queue it never left. The weekly-stats fallback
narrows its catch, so a failure that is not data unavailability propagates rather
than silently deriving the season from play-by-play.

cache — parquet and metadata both land through a temporary file and os.replace, so
a crash or a second writer leaves one whole file or the other. A size ceiling
evicts in least-recently-accessed order and orphans are reaped in both directions;
one parquet per season tuple had otherwise accrued without bound against
play-by-play measured in hundreds of megabytes.

seasons — `current_season` and `recent_seasons` resolve the nflverse season label
from an injectable date, replacing thirteen hardcoded season literals across the
six views that needed editing each September.

─── Gates ───

.github/workflows/ci.yml runs lint, format, typecheck, tests with coverage, build,
and a dependency vulnerability scan that fails the build on findings. Installs come
from the committed lockfile, so lockfile drift fails before any gate runs.

pyproject sets mypy `strict = true` with no carve-out, adds ten optional error
codes, and enables `ignore-without-code` and `warn_unused_ignores` so silencing a
check is itself a failure. Coverage measures src/ffb with branch coverage against a
floor taken from measurement.

tests/test_project_gates.py pins the gates against silent relaxation: any boolean
under [tool.mypy] set false fails, since a false there reads as strict while
exempting the check it names.

Views, the base class, both data-layer modules and the app shell gain suites,
driven headlessly through Textual's pilot with the loaders stubbed. The refresh
contract is parametrised across all six views so no tool can regress alone.

Co-authored-by: stxkxsbot <275011021+stxkxsbot@users.noreply.github.com>
…hape

Three documented behaviours did not match the code, and one named file did not
exist. A claim a reader can act on and be wrong about is worse than no claim.

─── Corrections ───

README described the trade-value production component as season PPG times games
remaining, weighted 0.40, alongside a separate 0.10 bye-week penalty. It documents
what the engine computes: production is PPG times per-player games remaining, which
subtracts a bye still ahead, weighted 0.50, with no standalone bye term.

README's injury-impact bullets keyed the confidence tier to a generic sample size
and named the wrong season window. Both are read from the engine and the view.

CLAUDE.md listed `nfl.import_player_stats`, which the installed nfl_data_py does
not export. The importer list names the seven the loaders call, each of which
resolves.

CLAUDE.md pointed at `nfl-data-api-research.md` for API research and decision
rationale. No such file is in the repository. The supplementary-API content it
promised already sits inline in CLAUDE.md, so the dangling pointer is removed
rather than satisfied with invented research.

─── Additions ───

README gains Configuration, Troubleshooting and Development sections. The first run
of any tool is a multi-minute download of two seasons of play-by-play, so
Troubleshooting names the elapsed-time readout, the Cancel button and escape
binding, the per-download timeout and what its failure means. Configuration states
the cache location, its TTL and its size ceiling, each read from the source.

docs/architecture.md explains the shape a contributor needs and the code cannot
state for itself: why each tool is a bounded context split into a pure engine and a
Textual view, why engines take frames and return frames with no I/O, what ToolView
owns against what a view owns, how the cache and loader sit underneath, and what
adding a seventh tool involves.

Co-authored-by: stxkxsbot <275011021+stxkxsbot@users.noreply.github.com>
@stxkxs
stxkxs marked this pull request as ready for review August 25, 2026 03:38
@stxkxs
stxkxs merged commit b1e7e17 into main Aug 25, 2026
2 checks passed
@stxkxs
stxkxs deleted the feature-check branch August 25, 2026 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant